Skip to content

tests: treat only deleted servers as reusable for remote URL validation#1204

Open
stationeros wants to merge 20 commits intomodelcontextprotocol:mainfrom
stationeros:main
Open

tests: treat only deleted servers as reusable for remote URL validation#1204
stationeros wants to merge 20 commits intomodelcontextprotocol:mainfrom
stationeros:main

Conversation

@stationeros
Copy link
Copy Markdown

@stationeros stationeros commented Apr 25, 2026

Added targeted service-layer test coverage for the main scenarios:

  • creating a server with a remote URL that matches a deleted server is allowed
  • creating a server with a remote URL that matches a deprecated server fails
  • restoring a deleted server to active still fails if another non-deleted server now uses that remote URL
  • restoring all deleted versions to active fails under the same conflict condition

@pree-dew
Copy link
Copy Markdown
Contributor

pree-dew commented Apr 25, 2026

@stationeros Thank you raising for the PR 👍 . Few points as per the design of status behaviour:

  • Remote url attached to a deprecated server is a valid URL. Deprecated server can still be an active server for downstream clients. It might not be maintained by the owner. If a user wants to publish a new package with this remote url, they have to mark this as deleted first.
  • Migrating from deleted to active and vice versa are the 2 status that require remote url validation.
  • If we are updating a server then also we have to check for remote url validation.

Comment thread internal/service/registry_service.go Outdated
@stationeros stationeros requested a review from pree-dew April 25, 2026 11:35
@stationeros stationeros changed the title fix: ignore deprecated servers in remote URL conflict checks fix: treat only deleted servers as reusable for remote URL validation Apr 25, 2026
Comment thread internal/service/registry_service_test.go Outdated
Comment thread internal/service/registry_service_test.go Outdated
@stationeros stationeros requested a review from pree-dew April 27, 2026 16:51
@pree-dew
Copy link
Copy Markdown
Contributor

@stationeros We might not need this change, except for test cases. As this behaviour is already part of code where the default value is to exclude deleted servers. If you will run the test cases without the filter change, you will notice that tests are passing.

Reason for the issue is different where status of servers are not marked as deleted. Apologies from my end, I missed checking the default values first. But we might need test cases though. I appreciate your contribution and patience here.

@stationeros
Copy link
Copy Markdown
Author

@stationeros We might not need this change, except for test cases. As this behaviour is already part of code where the default value is to exclude deleted servers. If you will run the test cases without the filter change, you will notice that tests are passing.

Reason for the issue is different where status of servers are not marked as deleted. Apologies from my end, I missed checking the default values first. But we might need test cases though. I appreciate your contribution and patience here.

nvm - reverted the logic and kept the TCs if that helps

@stationeros stationeros changed the title fix: treat only deleted servers as reusable for remote URL validation tests: treat only deleted servers as reusable for remote URL validation Apr 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants